Skip to content

Missing Test for InitContainers & EphemeralContainers GPU Quota Checks - #2367

Closed
aniket866 wants to merge 1 commit into
Project-HAMi:masterfrom
aniket866:calcualte-score
Closed

Missing Test for InitContainers & EphemeralContainers GPU Quota Checks#2367
aniket866 wants to merge 1 commit into
Project-HAMi:masterfrom
aniket866:calcualte-score

Conversation

@aniket866

@aniket866 aniket866 commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Path Under Test

  • webhook.go:L131-L178 (fitResourceQuota)

Explanation & Bug Gap

The admission webhook's quota controller only checks resource limits inside pod.Spec.Containers. If a pod runs an InitContainer requesting GPUs, the quota checks are bypassed entirely. The current tests in webhook_test.go only verify resource configurations within regular containers.

Why Add This Test

To verify that init containers and ephemeral containers cannot bypass namespace GPU quota constraints, ensuring proper resource isolation inside multi-tenant clusters.

Mermaid Diagram

graph TD
    A[Test Case: InitContainer with GPUs] --> B[fitResourceQuota]
    B --> C{Scan Containers, InitContainers}
    C --> D[Aggregate Total GPU Core/Memory]
    D --> E{Exceeds Quota?}
    E -- Yes --> F[Assert: Webhook Denies Admission]
    E -- No --> G[Assert: Webhook Allows Admission]
Loading

Proposed Test Implementation

Create a test inside webhook_test.go that mocks an admission request for a Pod where Spec.InitContainers requests a GPU count or memory limit exceeding the namespace resource quota. Assert that the admission response has Allowed: false and mentions exceeding the quota.


Summary by CodeRabbit

  • Bug Fixes

    • Prevented crashes when GPUs report asymmetric peer-to-peer link information.
    • Added a warning and safely returns a fallback score of zero for affected GPU pairs.
  • Documentation

    • Added a changelog entry documenting the fix in version 2.9.1.

Signed-off-by: aniket866 <iamaniketkumarmaner@gmail.com>
@hami-robot
hami-robot Bot requested a review from FouoF August 4, 2026 19:57
@hami-robot
hami-robot Bot requested a review from ouyangluwei163 August 4, 2026 19:57
@hami-robot

hami-robot Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: aniket866
Once this PR has been reviewed and has the lgtm label, please assign archlitchi for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: d65cb76a-1247-4ef2-aca6-c19721594dda

📥 Commits

Reviewing files that changed from the base of the PR and between 2cabe29 and b08299b.

📒 Files selected for processing (3)
  • CHANGELOG.md
  • pkg/device/nvidia/calculate_score.go
  • pkg/device/nvidia/calculate_score_test.go

📝 Walkthrough

Walkthrough

The GPU pair scoring path now handles asymmetric NVML P2P-link counts without panicking. It logs a warning and returns zero scores. A regression test covers both directions, and the changelog records the fix.

Changes

Asymmetric GPU link handling

Layer / File(s) Summary
Warning fallback and regression coverage
pkg/device/nvidia/calculate_score.go, pkg/device/nvidia/calculate_score_test.go, CHANGELOG.md
calculateGPUPairScore logs asymmetric link counts with GPU UUIDs and returns zero. The test validates both GPU directions. The changelog records the v2.9.1 fix.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related issues

Possibly related PRs

Suggested reviewers: wawa0210, ouyangluwei163

Poem

A rabbit found two GPUs in line,
One link showed, the other drew none.
“No panic,” said Bun, “log what we see,”
Then scored both paths at zero, neatly.
The changelog hopped after,
And tests thumped with laughter.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Title check ⚠️ Warning The title describes GPU quota tests for container types, but the changes fix asymmetric NVML GPU link handling and add related tests. Update the title to describe the asymmetric NVML GPU link panic fix and the added regression test.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Warning

⚠️ This pull request shows signs of AI-generated slop (description_diff_mismatch). It has been flagged by CodeRabbit slop detection and should be reviewed carefully.

@hami-robot hami-robot Bot added the size/M label Aug 4, 2026
@coderabbitai
coderabbitai Bot requested a review from wawa0210 August 4, 2026 19:57
@mesutoezdil

Copy link
Copy Markdown
Contributor

pls read and understand what are you doing with codes first

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants